Claude finished @dsolistorres's task in 5m 43s —— View job

🔍 dotCMS Backend Review

Tasks

• Load dotCMS conventions
• Get Java diff
• Run specialized sub-agent reviews (security, database, Java standards, REST API)
• Consolidate findings and post review

[🟠 High] dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp:47-64

Resource existence is disclosed before the authorization check. findContentletByIdentifierAnyLanguage is a privileged call that bypasses permissions; the JSP then returns 404 when the contentlet is not found, and only returns 403 after confirming it exists. Any backend-authenticated user with portlet access can enumerate valid content UUIDs by observing the difference between 404 and 403 responses.

// 404 is returned here — before permissions are evaluated
if (contentlet == null || !UtilMethods.isSet(contentlet.getIdentifier())) {
    throw new WebApplicationException(Response.status(Response.Status.NOT_FOUND)...);
}
// Permission check comes AFTER existence check
if (!APILocator.getPermissionAPI().doesUserHavePermission(...)) {
    throw new WebApplicationException(Response.status(Response.Status.FORBIDDEN)...);
}

💡 Use a permission-aware API call (e.g. contentletAPI.findContentletByIdentifier(id, user, lang)) which throws DotSecurityException on denied access, then map that to 403. This collapses the two branches so the caller cannot infer existence from the status code.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java:81

e.printStackTrace() remains in render(), a method in the same file that was modified by this PR. dotCMS standards require Logger for all diagnostic output; System.err output is invisible to the log management system and violates the progressive-enhancement rule that applies when a file is touched.

} catch (ServletException e) {
    e.printStackTrace();   // ❌ must be Logger.error(...)
}

💡 Replace with Logger.error(this.getClass(), "Failed to render portlet JSP", e);

[🟡 Medium] dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp:79

The session locale attribute is interpolated into a JavaScript string literal without encoding. Every other dynamic value on the page uses Xss.encodeForJavaScript(), but this one does not.

var langIsoCode = '<%= request.getSession().getAttribute(Globals.LOCALE_KEY) %>';

💡 Wrap with Xss.encodeForJavaScript(String.valueOf(...)) to match the encoding used for all other JS interpolation on the page.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java:144-148

getLayout has a @GET endpoint that sets Cache-Control: no-cache only on the happy-path response (line 187). The three early-exit error paths (500, 403, 500 at lines 167, 177, 182) return bare Response.status(N).build() with no cache-control header. A caching proxy could cache a 403 or 500 and serve it to subsequent legitimate requests.

ResponseBuilder builder = Response.status(500);
return builder.build();  // no Cache-Control header on error path

💡 Add @NoCache at the method level so the JAX-RS filter applies the header to every response including error paths.

Next steps

• 🟠 Fix locally and push — these need your judgment
• 🟡 You can ask me to handle mechanical fixes inline: @claude fix <issue description> in <File.java>
• Every new push triggers a fresh review automatically

🔍 dotCMS Backend Review

[🔴 Critical] dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java:136-145

Non-WebApplicationException errors (e.g. NullPointerException, IOException) in any JSP still follow the old code path: the method returns debug HTML from sw.toString(), which getLayout wraps as Response.ok(..., "text/html") — an HTTP 200 with internal error details in the body. The PR fixed WAE propagation but left all other exceptions returning a silent 200. The test getJspResponse_returnsErrorHtmlForGenericException explicitly asserts this behaviour, anchoring the gap in the test suite.

sw.append(e.toString());   // internal exception details
return sw.toString();      // returned as HTTP 200 by getLayout

💡 After the cause-chain loop, throw a WebApplicationException(500) instead of building the debug HTML, so callers always get a proper HTTP status. Remove or update the two "error HTML" unit tests to reflect the new contract.

[🟠 High] dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java:138,141

path (partially user-influenced via jspName from the URL) and e.toString() are interpolated into an HTML response without encoding. An attacker who can force a JSP-parse error with a crafted jspName containing HTML/JS metacharacters can inject markup into the admin-UI error page. This is a pre-existing path but it remains reachable for any exception that is not a WebApplicationException.

sw.append("unable to parse: <a href='" + path + "' target='debug'>" + path + "</a>");
sw.append(e.toString());  // may echo request-derived data; no HTML-encoding

💡 The cleanest fix is the 🔴 Critical fix above (throw 500 instead). If the debug HTML must be kept, use Encode.forHtmlAttribute(path) and Encode.forHtml(e.toString()).

[🟠 High] dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp:36

findContentletByIdentifierAnyLanguage takes no User argument and performs no permission check. A backend-authenticated user with no READ permission on a contentlet can probe for its existence by UUID and receive a 200 (vs. 404), leaking that the content exists. The init.jsp include only confirms a backend session is present.

// No user or permission argument — any backend session can probe
contentlet = APILocator.getContentletAPI().findContentletByIdentifierAnyLanguage(id);

💡 Resolve the user from the session and verify READ permission via PermissionAPI.doesUserHavePermission() before using the contentlet; throw 403 on denial.

[🟡 Medium] dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp:37-43

The catch block swallows all exception types — including DotSecurityException — and maps them to 400 BAD_REQUEST. A valid UUID whose owner has insufficient permissions returns the same status as a malformed request, masking real access-control failures and making it harder to distinguish input errors from permission/infra errors.

} catch (final Exception e) {
    throw new WebApplicationException(
        Response.status(Response.Status.BAD_REQUEST) ...
    );
}

💡 Catch DotSecurityException first and map to 403, then map remaining exceptions to 500. Reserve 400 for bad-input cases only.

[🟡 Medium] dotCMS/src/test/java/com/dotcms/rest/BaseRestPortletTest.java:120-121,143-144

javax.servlet.ServletException is referenced by its fully-qualified name inside both new test methods, while other javax.servlet.* types (e.g. RequestDispatcher) are imported at the top of the file. This is inconsistent with the rest of the class.

final javax.servlet.ServletException wrapped =
        new javax.servlet.ServletException("jsp failed", root);

💡 Add import javax.servlet.ServletException; and use the simple name.

Next steps

• 🔴 / 🟠 Fix locally and push — these need your judgment
• 🟡 You can ask me to handle mechanical fixes inline: @claude fix <issue description> in <File.java>
• Every new push triggers a fresh review automatically